Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configuration of the percentile metrics to submit in Datadog API reporter #1360

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

thyandrecardoso
Copy link
Contributor

Currently, only p95 gets reported. Looking at p99 seems a fairly common and important use case. I tested this by making sure that these changes work against our Datadog account and I see the new metric.

Can we report p99 as well?

@ivantopo
Copy link
Contributor

ivantopo commented Sep 2, 2024

I think that adding the p99 would be a good addition, but I wouldn't just add it as a default because that one additional metric could have a significant effect on people's bills because they are counted as custom metrics.

Could you please change the implementation so that we read a list of percentiles from config (similar to how the influxdb reporter does here: https://github.com/kamon-io/Kamon/blob/master/reporters/kamon-influxdb/src/main/resources/reference.conf#L16) and default to only the p95 so that it behaves the same after people upgrade?

@thyandrecardoso
Copy link
Contributor Author

Makes sense. Will try to improve this in the upcoming days...

@thyandrecardoso thyandrecardoso force-pushed the add-p99 branch 2 times, most recently from 5c29a25 to f086e14 Compare September 3, 2024 23:45
@thyandrecardoso
Copy link
Contributor Author

@ivantopo Hi, can you take a look at this again please?

@thyandrecardoso thyandrecardoso changed the title Add p99 metric for Datadog API reporter Allow configuration of the percentile metrics to submit in Datadog API reporter Sep 4, 2024
Copy link
Contributor

@hughsimpson hughsimpson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@hughsimpson hughsimpson merged commit b8dea69 into kamon-io:master Oct 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants